Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

auth: added a new config for direct queries of dnskey signature #14581

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

d-wibowo
Copy link
Contributor

Short description

Addresses the feature request that I made on Issue #14372.

Added a new setting called direct-dnskey-signature, which allows for the direct retrieval of DNSKEY signature from the backend.

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)
  • checked that this code was merged to master

@coveralls
Copy link

coveralls commented Aug 23, 2024

Pull Request Test Coverage Report for Build 12683621063

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 2 of 2 (100.0%) changed or added relevant lines in 2 files are covered.
  • 13925 unchanged lines in 171 files lost coverage.
  • Overall coverage increased (+5.9%) to 64.824%

Files with Coverage Reduction New Missed Lines %
pdns/auth-catalogzone.hh 1 66.67%
pdns/recursordist/sortlist.hh 1 75.0%
ext/lmdb-safe/lmdb-typed.cc 1 89.66%
pdns/test-dnsrecords_cc.cc 2 95.97%
ext/probds/murmur3.cc 2 94.12%
pdns/epollmplexer.cc 2 85.0%
pdns/dnssecinfra.cc 2 76.42%
pdns/json.cc 2 83.33%
pdns/recursordist/test-negcache_cc.cc 2 98.52%
pdns/recursordist/test-rec-tcounters_cc.cc 2 95.77%
Totals Coverage Status
Change from base Build 10516284382: 5.9%
Covered Lines: 126136
Relevant Lines: 163814

💛 - Coveralls

@Habbie Habbie added this to the auth-5 milestone Aug 26, 2024
@Habbie Habbie self-requested a review August 26, 2024 11:39
@d-wibowo
Copy link
Contributor Author

Hi @Habbie, any plans to review this soon?

@d-wibowo
Copy link
Contributor Author

Hi @Habbie , are there any news on this PR? Do you have any plans or schedules yet?

@Habbie
Copy link
Member

Habbie commented Oct 21, 2024

Can't promise anything but I really do hope to get to it soon.

@d-wibowo
Copy link
Contributor Author

d-wibowo commented Jan 7, 2025

Hi @Habbie, sorry but I'd like to ask again if you have any updates on this PR?

Copy link
Member

@Habbie Habbie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so sorry about the delay. I finally had a look. What you did looks good, but incomplete.

Adding a few tests would be great (regression-tests.auth-py makes it relatively easy to run a separate pdns with a specific configuration). One of those tests should be for AXFR, as I am unsure that code path also needs work.

The option also needs to be documented (docs/settings.rst)

@d-wibowo
Copy link
Contributor Author

d-wibowo commented Jan 8, 2025

@Habbie I've updated the docs already you can check it out.

As for the regression test, I'm gonna be honest with you I feel lost 😅 . I followed the README in regression-tests/ and managed to run it. But from what I understand, each of the tests under the tests/ directory uses the same pdns instance?

I've looked into regression-tests.auth-py/ and saw what you meant about having separate pdns configurations; they should correspond to each of the test_x.py files right?

And to run the tests do we just use the call the runtests script? I tried running the runtests script but never managed to succeed doing so. Any pointers on where I should start?

@miodvallat
Copy link
Contributor

I've looked into regression-tests.auth-py/ and saw what you meant about having separate pdns configurations; they should correspond to each of the test_x.py files right?

Every test_whatever.py file in there defines a test class with its own pdns configuration and zone data, and the various testWhatever methods will get invoked to perform tests on that configuration.

And to run the tests do we just use the call the runtests script? I tried running the runtests script but never managed to succeed doing so. Any pointers on where I should start?

Simply run ./runtests in regression-tests.auth-py/. But if you are only interested in a particular test*.py file, pass its name as argument to runtests and it will only run the tests from this file.

If you have a good idea of what kind of configuration data and requests to send, I can help you write these tests.

@d-wibowo d-wibowo requested a review from Habbie January 9, 2025 05:16
@d-wibowo
Copy link
Contributor Author

d-wibowo commented Jan 9, 2025

Thanks @miodvallat I managed to create a new .py file for a test and ran it with ./runtests test.py.

@Habbie could you check the latest commit I made? I added a couple of tests, however I didn't include AXFR since I'm not particularly familiar with it.

Also I'm not sure why one of the check here in github actions failed.. It's a test for test-recursor-regression (ubsan+asan, debian, 48) but I only modified the authpy test.

@miodvallat
Copy link
Contributor

Glad to have been of help. The test failure you have hit is an (annoying) known sporadic failure, I have restarted the failing test and hopefully the next run should be all green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants